-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: build & publish linux/arm64 Docker images #1367
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:
|
|
I think it would make sense to temporarily run the Docker build in this PR too, without publishing of course. What do you think? |
@derhuerst do you still want to get this checked in? You'll need to sign the CLA before we can do so. Regarding running the Docker build against this PR. I'll admit I'm not totally clear on how to do that, since the Docker workflow doesn't run for PRs by default. That said, this change seems mostly reasonable, so we could probably check it in as-is and rollback if it causes some issue with the Docker build? Or did you have a different concern in mind? |
branches: | ||
- master | ||
# todo: revert | ||
- docker-arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be reverted before merging.
@@ -132,7 +136,9 @@ jobs: | |||
file: ./Dockerfile | |||
build-args: | | |||
VERSION_TAG=${{ steps.prep.outputs.version }} | |||
push: true | |||
# todo: revert | |||
push: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be reverted before merging.
Am I reading correctly that this failed to run on your repo per https://github.com/derhuerst/gtfs-validator/actions/runs/4741545166/jobs/8418790748 ? Any thoughts? |
Not sure 🤷 |
There is a long running bug with Docker (docker/buildx#59) where multi-platform builds don't work with a
I think the solution here would be to only build a single platform for the test Docker build but then enable multi platforms for the push. Thoughts? |
Sounds good, as I'm mainly interested in having a native Docker image to pull and use. |
@derhuerst I created a draft PR with an alternative solution based on @bdferris-v2 comment. Can I get some feedback on #1528 ? Thanks! |
@@ -108,6 +111,7 @@ jobs: | |||
build-args: | | |||
VERSION_TAG=${{ steps.prep.outputs.version }} | |||
load: true | |||
platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid getting
ERROR: docker exporter does not currently support exporting manifest lists
you'll have to either delete this line, or create a third copy of the docker/build-push-action@v3
, (one with load, one with platforms, one with push).
Btw, why not set For reference, I ended up just rolling with FROM eclipse-temurin:17-jdk-jammy as valid-gtfs
COPY --from=ghcr.io/mobilitydata/gtfs-validator:4.1.0 gtfs-validator-cli.jar /
WORKDIR /work
COPY foo-gtfs.zip .
RUN java -jar /gtfs-validator-cli.jar -i /work/foo-gtfs.zip -o /work/out/ for running on ARM. Side note: The |
On an ARM64 system, I would like to use native Docker images in order not to run an emulation layer.
This PR extends the Docker build & publishing CI workflow to also build
linux/arm64
images in addition tolinux/amd64
images (implicit default).Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything